Created dynamic list of posts #916

davinnchii · 2023-09-19T09:28:22Z

roman-mirzoian

Good job, but there is quite a bit of work to be done.

roman-mirzoian · 2023-09-21T11:35:00Z

src/App.tsx

+      onClick={() => {
+        if (isDropDownActive) {
+          setIsDropDownActive(false);
+        }
+      }}


This callback should be moved to a separate handler.

roman-mirzoian · 2023-09-21T11:35:30Z

src/App.tsx

+                    Something went wrong!
+                  </div>
+                )}
+                {(selectedUser && !postsLoadingError) && (


For better readability, conditions where there are more than two values should be assigned to separate variables.

roman-mirzoian · 2023-09-21T11:36:25Z

src/components/NewCommentForm.tsx

+  const [name, setName] = useState('');
+  const [email, setEmail] = useState('');
+  const [commentText, setCommentText] = useState('');


If we want to have a managed state for one entity with simple inputs, it is better to keep everything in one state.

roman-mirzoian · 2023-09-21T11:38:57Z

src/components/NewCommentForm.tsx

+      createComment({
+        name,
+        email,
+        body: commentText,
+        postId: selectedPost.id,
+      }).then((newComment) => {
+        setComments(prev => [...prev, newComment]);
+      }).finally(() => {
+        setCommentText('');
+        setIsSubmitted(false);
+        setIsLoading(false);
+      });


Add a catch block to catch possible errors and (optional) try to use try...catch construction (I think this design is more convenient for long records).

roman-mirzoian · 2023-09-21T11:42:52Z

src/components/PostDetails.tsx

+          {isLoading ? (
+            <Loader />
+          ) : (
+            <>
+              {hasError ? (
+                <div className="notification is-danger" data-cy="CommentsError">
+                  Something went wrong
+                </div>
+              ) : (
+                <>
+                  {
+                    comments.length ? (
+                      <>
+                        <p className="title is-4">Comments:</p>


Everything works fine now, but it's written with a lot of nesting, so it's difficult to read and maintain in the future.
Try to avoid this by moving part of the code to the new Comments component.

roman-mirzoian · 2023-09-21T11:43:17Z

src/components/PostsList.tsx

+        <Loader />
+      ) : (
+        <div data-cy="PostsList">
+          {posts.length > 0 ? (


Suggested change

{posts.length > 0 ? (

{posts.length ? (

roman-mirzoian · 2023-09-21T11:44:16Z

src/components/UserSelector.tsx

+    getUsers()
+      .then(setAllUsers);


Add a catch block.

roman-mirzoian · 2023-09-21T11:44:34Z

src/components/UserSelector.tsx

+            >
+              {user.name}
+            </a>
+


Suggested change

Wesses

LGTM

BudnikOleksii

Looks great 🔥
Let's slightly improve your solution and fix some issues

BudnikOleksii · 2023-09-22T15:05:10Z

src/App.tsx

+  const [isNewCommentActive, setIsNewCommentActive] = useState(false);
+  const [isDropDownActive, setIsDropDownActive] = useState(false);
+
+  const [postsLoadingError, setPostsLoadingError] = useState(false);


isPostsLoadingError or just isError

BudnikOleksii · 2023-09-22T15:05:56Z

src/components/Comments.tsx

+              <article
+                className="message is-small"
+                data-cy="Comment"
+                key={comment.id}
+              >
+                <div className="message-header">
+                  <a href={`mailto:${comment.email}`} data-cy="CommentAuthor">
+                    {comment.name}
+                  </a>
+                  <button
+                    data-cy="CommentDelete"
+                    type="button"
+                    className="delete is-small"
+                    aria-label="delete"
+                    onClick={() => onDelete(comment.id)}
+                  >
+                    delete button
+                  </button>
+                </div>
+
+                <div className="message-body" data-cy="CommentBody">
+                  {comment.body}
+                </div>
+              </article>


Create a component Comment

BudnikOleksii · 2023-09-22T15:07:23Z

src/components/Contexts/CommentsContext.tsx

+export const useComments = () => {
+  const comments = useContext(CommentsContext);
+
+  return comments;
+};


Suggested change

export const useComments = () => {

const comments = useContext(CommentsContext);

return comments;

};

export const useComments = () => useContext(CommentsContext);

BudnikOleksii · 2023-09-22T15:08:43Z

src/components/NewCommentForm.tsx

+      const newComment = await createComment({
+        name,
+        email,
+        body: commentText,
+        postId: selectedPost.id,
+      });


Request to server must be inside try/catch block to catch server errors

BudnikOleksii · 2023-09-22T15:11:59Z

src/components/NewCommentForm.tsx

+  const handleChangeName = (event: React.ChangeEvent<HTMLInputElement>) => {
+    setCommentForm(prev => ({ ...prev, name: event.target.value }));
+  };
+
+  const handleChangeEmail = (event: React.ChangeEvent<HTMLInputElement>) => {
+    setCommentForm(prev => ({ ...prev, email: event.target.value }));
+  };
+
+  const handleChangeComment = (
+    event: React.ChangeEvent<HTMLTextAreaElement>,
+  ) => {
+    setCommentForm(prev => ({ ...prev, commentText: event.target.value }));
+  };


You can combine them into one handler. Input name you can get from event.target. Something like this

const handleChange = ( event: React.ChangeEvent<HTMLInputElement | HTMLTextAreaElement>, ) => { const { name, value } = event.target; setCommentForm(prev => ({ ...prev, [name]: value })); };

BudnikOleksii · 2023-09-22T15:20:15Z

src/components/PostDetails.tsx

+  const toggleAddCommentForm = () => {
+    onAddComment(true);
+  };

-          <article className="message is-small" data-cy="Comment">
-            <div className="message-header">
-              <a
-                href="mailto:misha@mate.academy"
-                data-cy="CommentAuthor"
-              >
-                Misha Hrynko
-              </a>
+  const handleDeleteComment = (commentId: number) => {
+    setComments(prev => prev.filter(({ id }) => id !== commentId));
+    onDeleteComment(commentId);
+  };


I didn't find where you catch API errors, if you don't catch an error your app can crash.
Also, filter comments only if the comment was successfully deleted

It is written in the task to filter comments before delete

Implement comment deletion
Delete the commnet immediately not waiting for the server response to improve the UX.

It is written in the task to filter comments before delete

Implement comment deletion
Delete the commnet immediately not waiting for the server response to improve the UX.

Sure, but you still need to handle possible API errors, and if actually comment wasn't added or deleted user should know about it. As it's weird if I add a comment, see it, then reload the page, and my comment disappears

BudnikOleksii · 2023-09-22T15:23:35Z

src/components/PostDetailsSidebar.tsx

+    setIsLoading(true);
+    if (selectedPost) {
+      getComments(selectedPost.id)
+        .then(setComments)
+        .catch(() => setHasError(true))
+        .finally(() => setIsLoading(false));
+    }


Suggested change

setIsLoading(true);

if (selectedPost) {

getComments(selectedPost.id)

.then(setComments)

.catch(() => setHasError(true))

.finally(() => setIsLoading(false));

}

if (selectedPost) {

setIsLoading(true);

getComments(selectedPost.id)

.then(setComments)

.catch(() => setHasError(true))

.finally(() => setIsLoading(false));

}

If selectedPost is null the isLoading state doesn't change to false.

But, as I see selectedPost is a required prop, so you can get rid of if (selectedPost) check

BudnikOleksii · 2023-09-22T15:25:02Z

src/components/PostDetailsSidebar.tsx

+            {`#${selectedPost?.id}: ${selectedPost?.title}`}
+          </h2>
+
+          <p data-cy="PostBody">
+            {selectedPost?.body}


Why do you need these checks selectedPost?, in your Props type this prop is required

BudnikOleksii · 2023-09-22T15:28:01Z

src/components/PostsList.tsx

+                <tbody>
+                  {posts.map((post) => {
+                    return (
+                      <tr data-cy="Post" key={post.id}>


Let's create a Post component

BudnikOleksii · 2023-09-25T08:28:33Z

src/App.tsx

+    setSelectedUser(null);
+    setSelectedUser(newUser);


Why do you set it to null, it doesn't work as expected in this case and makes no sense

BudnikOleksii · 2023-09-25T08:29:11Z

src/App.tsx

+                  <PostsList
+                    selectedPost={selectedPost}
+                    selectedUser={selectedUser}
+                    onSelectPost={(post) => setSelectedPost(post)}


Suggested change

onSelectPost={(post) => setSelectedPost(post)}

onSelectPost={setSelectedPost}

BudnikOleksii · 2023-09-25T08:31:50Z

src/components/NewCommentForm/NewCommentForm.tsx

+      }
+
+      setCommentForm(prev => ({ ...prev, commentText: '' }));
+      setIsSubmitted(false);
+      setIsLoading(false);


It should be done in the finally block

BudnikOleksii · 2023-09-25T08:36:18Z

src/components/PostDetails.tsx

+  const toggleAddCommentForm = () => {
+    onAddComment(true);
+  };

-          <article className="message is-small" data-cy="Comment">
-            <div className="message-header">
-              <a
-                href="mailto:misha@mate.academy"
-                data-cy="CommentAuthor"
-              >
-                Misha Hrynko
-              </a>
+  const handleDeleteComment = (commentId: number) => {
+    setComments(prev => prev.filter(({ id }) => id !== commentId));
+    onDeleteComment(commentId);
+  };


It is written in the task to filter comments before delete

Implement comment deletion
Delete the commnet immediately not waiting for the server response to improve the UX.

Sure, but you still need to handle possible API errors, and if actually comment wasn't added or deleted user should know about it. As it's weird if I add a comment, see it, then reload the page, and my comment disappears

BudnikOleksii

Good work 👍
But, you should handle API errors in any case.
Also, left a few comments above

BudnikOleksii · 2023-09-25T08:38:20Z

src/components/PostDetails/PostDetails.tsx

+  const toggleAddCommentForm = () => {
+    onAddComment(true);
+  };
+
+  const handleDeleteComment = (commentId: number) => {
+    setComments(prev => prev.filter(({ id }) => id !== commentId));
+    onDeleteComment(commentId);
+  };


I still don't see where you handle API errors

BudnikOleksii

Good work 👍

BudnikOleksii · 2023-09-25T09:11:10Z

src/components/PostDetails/PostDetails.tsx

+  const handleDeleteComment = (commentId: number) => {
+    setComments(prev => prev.filter(({ id }) => id !== commentId));
+    try {
+      onDeleteComment(commentId);
+    } catch {


Suggested change

const handleDeleteComment = (commentId: number) => {

setComments(prev => prev.filter(({ id }) => id !== commentId));

try {

onDeleteComment(commentId);

} catch {

const handleDeleteComment = async (commentId: number) => {

setComments(prev => prev.filter(({ id }) => id !== commentId));

try {

await onDeleteComment(commentId);

} catch {

onDeleteComment is an async operation so you should wait for the result.

For this task it's okay, but in real life, if something goes wrong you should notify the user and get back previous data, for example, if a comment wasn't added successfully, you should remove it from the UI. The same if the deletion fails you should get back the comment and show it, and notify a user that the error occurred

davinnchii added 3 commits September 18, 2023 17:29

Initial commit

cc986f3

Added state for new comment form

0ed913d

Fixed tests

c7a6fef

roman-mirzoian suggested changes Sep 21, 2023

View reviewed changes

Fix comments

5639d57

davinnchii requested a review from roman-mirzoian September 21, 2023 13:20

Wesses approved these changes Sep 21, 2023

View reviewed changes

BudnikOleksii suggested changes Sep 22, 2023

View reviewed changes

Fix comments

af14083

davinnchii requested a review from BudnikOleksii September 23, 2023 13:09

BudnikOleksii reviewed Sep 25, 2023

View reviewed changes

BudnikOleksii suggested changes Sep 25, 2023

View reviewed changes

fix comments

896c2d0

davinnchii requested a review from BudnikOleksii September 25, 2023 08:52

BudnikOleksii approved these changes Sep 25, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Created dynamic list of posts #916

Created dynamic list of posts #916

davinnchii commented Sep 19, 2023

roman-mirzoian left a comment

roman-mirzoian Sep 21, 2023

roman-mirzoian Sep 21, 2023

roman-mirzoian Sep 21, 2023

roman-mirzoian Sep 21, 2023

roman-mirzoian Sep 21, 2023

roman-mirzoian Sep 21, 2023

roman-mirzoian Sep 21, 2023

roman-mirzoian Sep 21, 2023

Wesses left a comment

BudnikOleksii left a comment

BudnikOleksii Sep 22, 2023

BudnikOleksii Sep 22, 2023

BudnikOleksii Sep 22, 2023

BudnikOleksii Sep 22, 2023

BudnikOleksii Sep 22, 2023

BudnikOleksii Sep 22, 2023

davinnchii Sep 23, 2023

BudnikOleksii Sep 25, 2023 •

edited

Loading

BudnikOleksii Sep 22, 2023

BudnikOleksii Sep 22, 2023

BudnikOleksii Sep 22, 2023

BudnikOleksii Sep 25, 2023

BudnikOleksii Sep 25, 2023

BudnikOleksii Sep 25, 2023

BudnikOleksii Sep 25, 2023 •

edited

Loading

BudnikOleksii left a comment

BudnikOleksii Sep 25, 2023

BudnikOleksii left a comment

BudnikOleksii Sep 25, 2023

	onSelectPost={(post) => setSelectedPost(post)}
	onSelectPost={setSelectedPost}

Created dynamic list of posts #916

Are you sure you want to change the base?

Created dynamic list of posts #916

Conversation

davinnchii commented Sep 19, 2023

roman-mirzoian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wesses left a comment

Choose a reason for hiding this comment

BudnikOleksii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BudnikOleksii Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BudnikOleksii Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

BudnikOleksii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BudnikOleksii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BudnikOleksii Sep 25, 2023 •

edited

Loading

BudnikOleksii Sep 25, 2023 •

edited

Loading